Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lint for parameters being used in actions to which they don't apply #3034

Merged
merged 7 commits into from
Apr 9, 2024
Merged

Lint for parameters being used in actions to which they don't apply #3034

merged 7 commits into from
Apr 9, 2024

Conversation

kichristensen
Copy link
Contributor

What does this change

Update linter to check if parameters are being used in actions to which they don't apply, as it would cause a runtime failure.

If a parameter is used in an action to which is doesn't apply porter lint will produce the output similar to this:

error(porter-101) - Parameter does not apply to action
install: 1st step in the exec mixin (Install Hello World)
Parameter onlyAppliesToUninstall does not apply to install action
See https://porter.sh/docs/references/linter/#porter-101 for more information
---

What issue does it fix

Closes #1018

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@kichristensen
Lint for parameters being used in actions to which they don't apply
97ca665 
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen kichristensen marked this pull request as ready for review March 25, 2024 12:56
@schristoff
Copy link
Member

My initial thought is: "Cool, but are we breaking backward compat with this?" - which I think is a a big yes, but I think it may be okay to do it in this case.

@kichristensen
Copy link
Contributor Author

@schristoff Completely forgot about backwards compability with this change. That said, in my opinion, it is not fully backwards compatible.
But as is causes a hard runtime error instead for failing on during build, I think it is acceptable, and even better to fail early.
It is a grey zone 🤷

No matter what happens with this PR, there should probably be some sort of strategy for adding new lints, as all new lints can be considered breaking changes in the sense that they potentially breaks existing builds, at least for errors. Might be worth taking up on a community meeting.

@sgettys
Copy link
Contributor

sgettys commented Mar 27, 2024

I agree it's better to hit this at build time than runtime. Any breakages because should only be on already broken bundles I would imagine

sgettys
sgettys reviewed Mar 27, 2024
View reviewed changes
pkg/linter/linter.go Outdated
}
results = append(results, res...)
}
span.EndSpan()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a defer otherwise this won't get closed if the return on 208 is hit. Any reason to have 2 different spans in here instead of a single one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, in both cases. Also I cannot find any good reasons for 2 spans here. I will change it

@sgettys
Copy link
Contributor

sgettys commented Mar 27, 2024

Can you add an integration test for this as well?

@kichristensen
Revert back to a single span
a37d7c4 
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen
Add integration test
3cafb67 
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen
Copy link
Contributor Author

Can you add an integration test for this as well?

@sgettys Sure, it has been added

@kichristensen kichristensen mentioned this pull request Apr 7, 2024
Merged
4 tasks
@schristoff
Copy link
Member

Since our next release is 1.1.0 (I know, breaking compat should be 2.0+ but this is a small compat thing) I'm fine with merging this sooner rather than later.

schristoff
schristoff approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@schristoff schristoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good once conflicts are resolved

@kichristensen
Merge branch 'main' into lintParamApplyTo
d9b701a 
Signed-off-by: Kim Christensen <2461567+kichristensen@users.noreply.github.com>
@schristoff schristoff merged commit 4f540b8 into getporter:main Apr 9, 2024
15 checks passed
@kichristensen kichristensen deleted the lintParamApplyTo branch April 10, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schristoff schristoff schristoff approved these changes

@bdegeeter bdegeeter Awaiting requested review from bdegeeter bdegeeter is a code owner

@troy0820 troy0820 Awaiting requested review from troy0820 troy0820 is a code owner

@sgettys sgettys Awaiting requested review from sgettys sgettys is a code owner

Assignees
No one assigned
Labels
None yet
Projects
v1.1.0 (June 2024)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint for parameters being used in actions to which they don't apply
3 participants
@kichristensen @schristoff @sgettys